Skip to content

Conversation

@mtpr-ot
Copy link
Contributor

@mtpr-ot mtpr-ot commented Dec 28, 2020

It ticker operation is initiated from e.g. hci tx thread, there is a
risk that the ticker job is run (without chaining) while it is already
running, triggered via mayfly.
To prevent this, the ticker instance job_guard is checked to prevent
re-entrance.
Clearing of the job_guard is moved down to complete all data
manipulating operations before allowing job to run again.

Signed-off-by: Morten Priess [email protected]

It ticker operation is initiated from e.g. hci tx thread, there is a
risk that the ticker job is run (without chaining) while it is already
running, triggered via mayfly.
To prevent this, the ticker instance job_guard is checked to prevent
re-entrance.
Clearing of the job_guard is moved down to complete all data
manipulating operations before allowing job to run again.

Signed-off-by: Morten Priess <[email protected]>
@mtpr-ot mtpr-ot force-pushed the bugfix/ticker_job_guard branch from 72fe5d2 to 00bbd46 Compare December 28, 2020 12:47
return;
}

/* Prevent running job if already running (re-entrance) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the need for running ticker_job from more than one execution/ISR/thread/task context? What is making ticker_job to pre-empt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the need for running ticker_job from more than one execution/ISR/thread/task context? What is making ticker_job to pre-empt?

We observe it with meta-IRQ threads.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct my understanding, a meta-IRQ thread should not pre-empt itself, right?

If you agree to the changes here being a workaround for issue with meta-IRQ, then the changes here can be under a workaround Kconfig option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, meta-IRQs run to completion - but pre-empts HCI thread (the debugging is a few months back, so I may need to brush up on the actual flow of things). A workaround config is OK with me, if we can make sure this is not a more general problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the issue occurs when ticker operations are called from thread context and ticker_job is executed inline instead of being chained. An example for this would be a call to ticker_stop from thread context that is preempted by an ISR that then calls, e.g., ticker_update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticker operations are called from thread context and ticker_job is executed inline instead of being chained

In this case, TICKER_USER_ID_THREAD is equal to TICKER_USER_ID_ULL_LOW to execute the ticker_job inline.

ticker_stop from thread context that is preempted by an ISR that then calls, e.g., ticker_update

Here ISR context is not equal to TICKER_USER_ID_ULL_LOW (ISR context has higher priority in comparison to the thread), and hence ticker_job shall not be a inline function when called by ticker_update.

What is important is, there shall only be one thread (or meta-IRQ) calling mayfly_run(TICKER_USER_ID_ULL_LOW); in the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are investigating whether changing the TICKER_USER_ID_XX "priorities" will solve the problem. This may take a while.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtpr-ot I see that I have faced the similar issue to which I propose this: #32752

@cvinayak cvinayak added this to the v2.6.0 milestone Jan 28, 2021
@cvinayak cvinayak added the Waiting for response Waiting for author's response label Jan 28, 2021
@mtpr-ot
Copy link
Contributor Author

mtpr-ot commented Mar 2, 2021

Closed as duplicate of #32752

@mtpr-ot mtpr-ot closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants